Implementing string and byte[] async response handlers. Porting over … #161

shorea · 2017-09-14T21:53:43Z

…temp fixes so PutObject will work

millems · 2017-09-14T21:56:10Z

core/src/main/java/software/amazon/awssdk/async/AsyncResponseHandler.java

+     * @param <ResponseT> Pojo response type.
+     * @return AsyncResponseHandler instance.
+     */
+    static <ResponseT> AsyncResponseHandler<ResponseT, String> dumpToString() {


why "dump"?

toString was taken. I can rename it to toUtf8String and have a toString that takes a Charset though per your other suggestion.

millems · 2017-09-14T22:00:08Z

core/src/main/java/software/amazon/awssdk/async/ByteArrayAsyncResponseHandler.java

+
+        @Override
+        public void onNext(ByteBuffer byteBuffer) {
+            invokeSafely(() -> baos.write(BinaryUtils.copyBytesFrom(byteBuffer)));


I must be going crazy because I can't find that this method actually exists. http://docs.oracle.com/javase/8/docs/api/?java/io/ByteArrayOutputStream.html What am I missing?

Do we need to copyBytesFrom, or can we just use array since the output stream copies it?

Not sure if we can assume it's backed by an array or that array constitutes the entire contents of the byte buffer.

Yeah, I guess we would need to check if it hasArray and preserve the indices if we were to do it.

millems · 2017-09-14T22:05:19Z

core/src/main/java/software/amazon/awssdk/async/StringAsyncResponseHandler.java

+
+    @Override
+    public String complete() {
+        return new String(byteArrayResponseHandler.complete(), StandardCharsets.UTF_8);


Because this is their data, we should probably make them define the encoding.

+1, I'll add that.

millems · 2017-09-14T22:08:46Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+
+        if (msg instanceof StreamedHttpResponse) {
+            requestContext.handler().onStream(new PublisherAdapter((StreamedHttpResponse) msg, channelContext, requestContext));
+        } else if (msg instanceof FullHttpResponse) {


Does this fix all of the remaining empty-response integration test failures?

Yeah, should fix them.

millems · 2017-09-14T22:28:24Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

+    }
+
+    @SafeVarargs
+    private static void removePerRequestHandlers(ChannelHandlerContext ctx, Class<? extends ChannelHandler>... handlerClasses) {


Does this need to be varargs anymore? I only see one use. It could even be inlined?

Nah I can inline it. Copied it from my other branch which removes some timeout handlers but didn't port that over.

millems · 2017-09-14T22:29:11Z

...nio-client/src/main/java/software/amazon/awssdk/http/nio/netty/internal/ResponseHandler.java

@@ -134,20 +196,18 @@ public void onNext(HttpContent httpContent) {

                @Override
                public void onError(Throwable t) {
-                    subscriber.onError(t);
+                    runAndLogError("Subsriber#onError threw an exception", () -> subscriber.onError(t));


Subsriber -> Subscriber (here and elsewhere). Will the subscriber be the customer's? Could it be beneficial to log which one?

Yes in this context it will be the customers subscriber. Were you thinking like a class name or something?

Yeah, a class name or just calling toString on it. We could implement useful toStrings for our internal implementations?

+1 for toString, I'll do that.

millems · 2017-09-15T19:02:26Z

core/src/main/java/software/amazon/awssdk/async/AsyncResponseHandler.java

+     * Creates an {@link AsyncResponseHandler} that writes all content to UTF8 encoded string.
+     *
+     * @param <ResponseT> Pojo response type.
+     * @return AsyncResponseHandler instance.


@param charset + other javadoc changes

millems

LGTM other than the minor other comments.

millems · 2017-09-15T19:04:59Z

core/src/main/java/software/amazon/awssdk/async/StringAsyncResponseHandler.java

+ * @param <ResponseT> Pojo response type.
+ */
+@SdkInternalApi
+class StringAsyncResponseHandler<ResponseT> implements AsyncResponseHandler<ResponseT, String> {


toString()?

millems · 2017-09-15T19:05:10Z

core/src/main/java/software/amazon/awssdk/async/ByteArrayAsyncResponseHandler.java

+ * @param <ResponseT> Pojo response type.
+ */
+@SdkInternalApi
+class ByteArrayAsyncResponseHandler<ResponseT> implements AsyncResponseHandler<ResponseT, byte[]> {


toString()?

It's actually the subscriber we call toString on. Since the byte subscriber didn't have any identifying state I just left it as the default implementation. I can change it to whatever though, just let me know.

Nah, it's aight.

…temp fixes so PutObject will work

millems reviewed Sep 14, 2017

View reviewed changes

shorea force-pushed the shorea-async-response-handler-string branch 3 times, most recently from b4d4bb5 to 1376635 Compare September 15, 2017 18:26

millems reviewed Sep 15, 2017

View reviewed changes

millems approved these changes Sep 15, 2017

View reviewed changes

shorea force-pushed the shorea-async-response-handler-string branch from 1376635 to bba549c Compare September 15, 2017 19:23

Implementing string and byte[] async response handlers. Porting over …

5ff3309

…temp fixes so PutObject will work

shorea force-pushed the shorea-async-response-handler-string branch from bba549c to 5ff3309 Compare September 15, 2017 19:26

Fixes for tests

6ebedbe

spfink approved these changes Sep 15, 2017

View reviewed changes

shorea merged commit 0f6a19d into master Sep 15, 2017

zoewangg deleted the shorea-async-response-handler-string branch December 6, 2017 19:15

Implementing string and byte[] async response handlers. Porting over … #161

Implementing string and byte[] async response handlers. Porting over … #161

Uh oh!

Conversation

shorea commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

millems Sep 15, 2017 •

edited

Loading